Support partial downloads #1020
Conversation
* Adds support only for the Curl backend, which is the default anyway. * In order to distinguish between a file that has been fully downloaded (but not used yet) and should therefore be hash-checked vs. those that require more data, store partials with a .partial extension. * Adds a simple http-server to rustup-mock, to allow the download module to be properly tested. It's not clear how to easily emulate a server that stops half-way without that. The tests for the overall download-resumption functionality should be fairly re-usable if we migrate to another download solution in the future (e.g. in rust-lang#993) * Don't bother with resumption for meta-data files, since they're likely to go out of date anyway.
Allows the removal of http test server, and simplifies test cases as we can just read/write files on disk, like for existing dist tests
…hes, and make consistent with partial download code.
|
Not sure about that travis error: "couldn't find crate for std." I don't know whether there's a way to just give it another kick? |
|
I restarted that job. The appveyor issues are legit though. |
I don't know why I put it there in the first place
|
Yes, that could definitely never work. Don't know why I put the conditional there. I can't check my work though, don't have a windows box, but think I've just pushed a fix. |
|
Thanks @jelford While it would be nice to move the download code out of rustup, I'm not sure that the intent is to switch to curl though? I'm not necessarily opposed to adding this kind of feature just for the curl backend though if it will help people on poor connections - @brson thoughts? |
|
Thanks for the quick response @Diggsey. On switching to curl, I just meant to observe that this is the default, so most people should get the benefit right away. I'm hoping 90% of this goes away as part of #993, so I wasn't keen to spend too long getting it into the hyper-backend. As a side note, I only took a quick look and I don't think reqwest currently has support for resume as a 1st-class thing. I was thinking it's easy enough just to set the appropriate header, but need to think about the error case where the server doesn't support it. libcurl will just bomb out when it gets a response without the Content-Range header. Is that actually fine? Do all the mirrors support it? It works for me locally, but I don't know whether it's S3 everywhere or something else in different parts of the world. If not, it would be a shame to break the whole thing for what is essentially a bandwidth optimization. Options in that case would be:
The former is simple, but not as nice a UI. |
My intent is to switch to reqwest, assuming that reqwest uses native-tls, and someday gets support for rustls. The rustup download crate was supposed to basically be reqwest, but at this point it seems best to do the hyper/rustls transition in the ecosystem, not in rustup. Adding resume support just means we'll have to make reqwest support that too (or otherwise create a crate that does what reqwest does with resume support). |
|
Good tests, thanks @jelford. |
I think we can not assume much about the servers, because people will at some point host their own rustup mirrors. |
|
Patch looks good to me, r=me if @Diggsey is happy. |
|
@bors r=brson |
|
📌 Commit c63b275 has been approved by |
Support partial downloads This PR should close #889 A couple of implementation details: Only added support for the curl backend; previously discussed that there's an intention to get rid of rustup's own download code, and the default feature-set uses curl anyway, so hopefully this is okay. Added new testing to the download crate - while it's there, it makes sense to have a test. Since using curl's "resume" functionality, I figured it's probably fine to just file:// urls for test cases. Previously tested using a small hyper-based http server, but that feels like overkill. For hashing files, I've set the buffer size to 2^15 - just because that's what strace tells me is used by `sha256sum` on my local PC. It seems much slower than that command though, and it's not obvious why, so maybe I've done something silly here. Finally, and maybe most controversially, I haven't done anything about cleaning up aborted partials. I don't really know when a good time is to do this, but a couple of suggestions that I'd be happy to implement: * Every run, just check the download cache for any files > 7 days old and smoke them * On self-update, as that seems like a natural time for generic "maintenance" sorts of operations I mentioned in my last PR, but the same disclaimer: I haven't written much rust, so I fully expect you will see some problems (also very happy to accept style criticisms). I accidentally ran a `rustfmt` on some things so apologies for the noise (can revert but... maybe it's worth having anyway?).
|
💔 Test failed - status-appveyor |
|
Windows flakiness |
This PR should close #889
A couple of implementation details:
Only added support for the curl backend; previously discussed that there's an intention to get rid of rustup's own download code, and the default feature-set uses curl anyway, so hopefully this is okay.
Added new testing to the download crate - while it's there, it makes sense to have a test. Since using curl's "resume" functionality, I figured it's probably fine to just file:// urls for test cases. Previously tested using a small hyper-based http server, but that feels like overkill.
For hashing files, I've set the buffer size to 2^15 - just because that's what strace tells me is used by
sha256sumon my local PC. It seems much slower than that command though, and it's not obvious why, so maybe I've done something silly here.Finally, and maybe most controversially, I haven't done anything about cleaning up aborted partials. I don't really know when a good time is to do this, but a couple of suggestions that I'd be happy to implement:
I mentioned in my last PR, but the same disclaimer: I haven't written much rust, so I fully expect you will see some problems (also very happy to accept style criticisms). I accidentally ran a
rustfmton some things so apologies for the noise (can revert but... maybe it's worth having anyway?).